-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
port position test to scalar #9128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @Lordworms . Could you delete position.slt
?
sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks @Lordworms
Since touching that, may I ask you to add tests with nulls and bad scenarios
select position(null in null)
select position('' in '')
select position(1 in 1)
and different other combinations in this direction
sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @Lordworms and @Weijun-H !
@comphead let us know what you think of this PR and if it is ready to merge |
Which issue does this PR close?
Closes #.
Rationale for this change
modify for #8922
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?